-
Notifications
You must be signed in to change notification settings - Fork 48
feat: OPCM v2 design doc #353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This PR adds the design document for OPCM v2.
| struct GameConfig { | ||
| GameType gameType; | ||
| address implementation; | ||
| bytes32 prestate; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since proofs contracts are now MCP-compatible, won't there be a 1:1 mapping between game type and implementation address, so we can remove the implementation arg?
| - **Add game types**: Implicit in `upgrade()` with modified games array | ||
| - **Update prestates**: Implicit in `upgrade()` with modified games array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about preserving addGameType and updatePrestates as convenience functions? We often want to do just those actions (e.g. changing prestate for a hard fork) and it feels error prone and hard to review to pass a full Config into upgrade for that. These convenience functions would just gather the other inputs before calling upgrade
| **Mitigation**: | ||
| - Comprehensive security audit with focus on the new flexible interfaces | ||
| - Defensive coding practices throughout implementation | ||
| - State diff review for the first OPCM v2 upgrade to verify correctness |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who needs to do state diff review—all signers, or just the devs creating the superchain-ops task?
| **Mitigation**: | ||
| - Ensure all existing OPCM v1 tests pass with v2 implementation | ||
| - Comprehensive test coverage mapping v1 functionality to v2 | ||
| - Review session with all customer teams to validate their use cases are covered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like an easy way to mitigate this is by not changing any external function signatures until the very end
| struct GameConfig { | ||
| GameType gameType; | ||
| address implementation; | ||
| bytes32 prestate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to specify prestate and gather everything up here or should we just take gameType, implementation and gameArgs to match the DisputeGameFactory.setImplementation constructor?
There's no requirement that a game type has exactly 1 prestate or that it be the only input to that type. The permissioned game probably shouldn't have a prestate at all and ZK may wind up needing more than one "prestate". Really the idea of a "prestate" only applies to the bisection process of FDG - it is the agreed state prior to the start of what's being bisected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From discussion: OPCM still only supports a fixed set of game types and knows how to convert from the particular config for that game type (which can be customised for it) to the game args needed (mixing in addresses for things like DelayedWETH etc).
| - Clear the `initialized` slot (for upgrades) | ||
| - Call the standard `initialize()` function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For upgrade we'd clear the initialised slot, for deploy we'd create the proxy right?
| - Solidity function signatures/ABIs are the source of truth | ||
| - Go code for constructing function calls is automatically generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we plan to do this generation? We've been trying to move away from the Geth codegen tool because it creates some pretty awful code and is quite limiting in how you can actually make calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've also seen issues where the struct definition is updated but the new fields aren't actually populated. We have some static analysis for this in op-deployer now but worth thinking about how we can ensure we get compiler errors if things are out of sync (including verifying the code was regenerated when required).
|
|
||
| Both functions begin by gathering the complete set of contracts for the chain—the "chain world"—which includes SystemConfig, OptimismPortal, AnchorStateRegistry, and all other chain contracts. | ||
|
|
||
| The load-or-deploy pattern attempts to load each contract address from an expected source (e.g., a known deployment address for upgrades, or a predictable location). If the contract cannot be found at the expected location, it is deployed instead. This unified pattern means the same chain world loading function serves both deploy and upgrade operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this deploying proxies only or also the implementation contracts? ie do we still depend on op-deployer boostrap implementations to actually deploy the implementation contracts?
| - **Upgrades**: Only a subset of configuration is provided in the upgrade input; the remaining configuration is gathered automatically from the existing deployed contracts | ||
|
|
||
| This asymmetry allows upgrades to specify only what changes, while deployments must provide complete configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this expressed in code? How do you know what you need to provide when calling upgrade? The ABIs above pass the same Config object to both deploy and upgrade so it seems unclear how to know if an upgrade requires a new setting be specified or not.
| - **Deploy new chains**: `deploy()` function | ||
| - **Upgrade existing chains**: `upgrade()` function | ||
| - **Dynamic game types**: GameConfig array in Config struct | ||
| - **Add game types**: Implicit in `upgrade()` with modified games array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there isn't a way to iterate through the list of set dispute game implementations (it's a map). Apart from migrating to interop, OPCM doesn't support removing game types, but it's worth noting that upgrade won't remove existing game types if they aren't in the games array. That feels a little more surprising with this ABI but I don't think it's unreasonable.
You can remove games by directly calling DisputeGameFactory.setImplementation and setting a 0x00 implementation address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the same thing applies to any maps set as part of initialize methods. That could open up some foot guns because map entries get left around after an upgrade and we don't have a way to specify that they should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From discussion: For game types, we have a fixed list that is supported so they can just be removed upfront and then re-add what is specified to make this work.
For the generic case we should ensure that initialisers only set simple fields and don't write to maps so they are safe to call repeatedly.
| bytes32 prestate; | ||
| } | ||
|
|
||
| struct Config { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we add a new field/change an existing field on the Config struct, won't we still have to update OP Deployer to plumb that field through the intent and into the OPCM? Is the idea that the codegen would make this process easier?
| Required reviewers: | ||
| - **Proofs team representative**: Primary stakeholder for interface flexibility requirements | ||
| - **Protocol team lead**: For overall architectural alignment | ||
| - **EVM Safety team representative**: For security and safety validation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need somebody from platforms as well to make sure it'll integrate well with deployer.
| ### Architecture Overview | ||
|
|
||
| OPCM v2 consolidates the 5 separate functions of v1 into 2 primary functions: | ||
| - `deploy(Config memory config)`: Deploys a new OP Stack chain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this include deploying blueprints?
|
|
||
| **Phase 1: Chain World Assembly (Load-or-Deploy Pattern)** | ||
|
|
||
| Both functions begin by gathering the complete set of contracts for the chain—the "chain world"—which includes SystemConfig, OptimismPortal, AnchorStateRegistry, and all other chain contracts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this include the Superchain contracts? How do we upgrade those?
|
One other thing - what's the rollout plan here? Would love for us to validate that this solves the Proofs team's problems as early as possible. |
This PR adds the design document for OPCM v2.